Revert "Revert "[relay] Update GO version and QUIC version (#4736)" (…#5071
Revert "Revert "[relay] Update GO version and QUIC version (#4736)" (…#5071
Conversation
📝 WalkthroughWalkthroughComprehensive toolchain and code modernization upgrade: Go bumped from 1.24 to 1.25, quic-go updated to v0.55.0, legacy Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.github/workflows/golang-test-freebsd.yml:
- Around line 28-31: The workflow sets GO_TARBALL/GO_URL to
"go1.25.3.freebsd-amd64.tar.gz"/"$GO_TARBALL" which mismatches release.yml's
go1.25.5; update the GO_TARBALL and any references to use go1.25.5 (e.g.,
"go1.25.5.freebsd-amd64.tar.gz" and corresponding GO_URL) so both workflows use
the same Go patch release, or add a comment/documentation near the
GO_TARBALL/GO_URL variables explaining and justifying the intentional version
difference.
In @.github/workflows/wasm-build-validation.yml:
- Line 30: The workflow pins golangci-lint-action to commit
4afd733a84b1f43292c63897423277bb7f4313a9 while commenting v8.0.0; update the
uses entry to point to the actual v8.0.0 commit hash
25e2cdc5eb1d7a04fdc45ff538f1a00e960ae128 (i.e., replace the existing commit in
the uses value) or, if you intentionally want a different commit, change the
trailing comment to the correct version tag to avoid the mismatch.
In @client/cmd/login.go:
- Line 84: Replace the generic `//nolint` on the line containing the `//nolint`
token with a rule-specific nolint (e.g., `//nolint:lll` for long lines) so only
the intended linter check is suppressed; identify the exact lint being avoided
by running your linter or checking similar instances (e.g., `client/cmd/root.go`
and `client/cmd/up.go` use `//nolint:lll`) and change the comment to
`//nolint:<rule>` to narrowly scope the suppression.
🧹 Nitpick comments (13)
management/server/store/sql_store_get_account_test.go (1)
1000-1005: LGTM! Good refactoring to use switch statement.The refactoring from if-else to switch improves readability and follows Go idioms for multiple equality comparisons against the same variable. The behavior remains functionally equivalent.
client/ssh/server/jwt_test.go (1)
605-616: Consider adding a default case for defensive coding.The switch statement correctly handles the current test cases. However, adding a default case that fails the test for unexpected token values would catch potential bugs if someone adds a new test case with a different token string value.
♻️ Suggested improvement
var authMethods []cryptossh.AuthMethod switch tc.token { case "valid": token := generateValidJWT(t, privateKey, issuer, audience) authMethods = []cryptossh.AuthMethod{ cryptossh.Password(token), } case "invalid": invalidToken := "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.invalid" authMethods = []cryptossh.AuthMethod{ cryptossh.Password(invalidToken), } + case "": + // Empty token case - authMethods remains empty + default: + t.Fatalf("unexpected token value in test case: %q", tc.token) }management/server/posture_checks.go (1)
161-161: Consider removing redundant.Error()call.When using the
%vformat verb with an error type, Go automatically calls the.Error()method. Explicitly callingerr.Error()is redundant and less idiomatic.♻️ Proposed refactor
- return status.Errorf(status.InvalidArgument, "%v", err.Error()) //nolint + return status.Errorf(status.InvalidArgument, "%v", err) //nolintclient/cmd/up.go (1)
219-219: Consider using a specific lint suppression directive.The generic
//nolintsuppresses all linter warnings for this line. It's better practice to specify which rule you're suppressing (e.g.,//nolint:lllfor line length or//nolint:errorlintfor error wrapping). This makes the code more maintainable and prevents accidentally hiding other issues.client/cmd/status.go (1)
127-127: Consider using a specific lint suppression directive.The generic
//nolintsuppresses all linter warnings. Specify which rule you're suppressing (e.g.,//nolint:lll,//nolint:errorlint) to make the intent clearer and prevent hiding unrelated issues.management/server/account.go (1)
1009-1009: Prefer specific lint suppression or fix the underlying issue.The change from
//nolint:gosimpleto//nolintmakes the suppression less specific, potentially hiding other warnings. Consider either:
- Reverting to
//nolint:gosimpleto be explicit about what's being suppressed- Fixing the code to eliminate the need for suppression by removing the redundant comparison
♻️ Option 2: Remove redundant comparison
- if datum.AppMetadata.WTPendingInvite != nil && *datum.AppMetadata.WTPendingInvite && loggedInOnce == true { //nolint + if datum.AppMetadata.WTPendingInvite != nil && *datum.AppMetadata.WTPendingInvite && loggedInOnce {client/cmd/root.go (1)
393-393: Consider using a specific lint suppression directive.The generic
//nolintsuppresses all linter warnings. Specify the rule being suppressed (e.g.,//nolint:lll,//nolint:errorlint) to improve code clarity and prevent accidentally hiding other issues.management/internals/shared/grpc/loginfilter_test.go (1)
88-89: Consider specifying the linter in the nolint directive.The bare
// nolintsuppresses all linters. For maintainability and clarity, specify which linter is being suppressed (e.g.,//nolint:gosecor//nolint:mndfor magic number detection).♻️ Suggested improvement
- // nolint + //nolint:mnd // exponential backoff calculation requires float arithmetic expectedSecondDuration := time.Duration(float64(baseBan) * math.Pow(2, 1))client/cmd/debug.go (1)
139-140: Consider removing the trailing newline from the error message.Error messages conventionally don't include trailing newlines since they're often wrapped or logged with additional formatting. The
//nolintdirective also lacks a linter specification.♻️ Suggested improvement
- //nolint - return fmt.Errorf("unknown log level: %s. Available levels are: panic, fatal, error, warn, info, debug, trace\n", args[0]) + return fmt.Errorf("unknown log level: %s. Available levels are: panic, fatal, error, warn, info, debug, trace", args[0]).devcontainer/Dockerfile (1)
1-12: Consider pinning gopls to a specific version for reproducibility.The base image and package updates align well with the Go 1.25 upgrade. However, changing gopls from a pinned version (
v0.18.1) to@latestmay cause inconsistent development environments over time as gopls releases new versions.♻️ Suggested improvement
- && go install -v golang.org/x/tools/gopls@latest + && go install -v golang.org/x/tools/gopls@v0.18.1Or update to a specific newer version that's compatible with Go 1.25.
client/cmd/login.go (1)
84-84: Consider using specific nolint directives.Both
//nolintcomments suppress lint warnings without specifying which rule. This can inadvertently suppress multiple rules. Use//nolint:rulenameto be explicit about which rule should be suppressed.Also applies to: 210-210
shared/relay/client/manager_test.go (1)
16-24: Inconsistency: ExposedAddress format differs from client_test.go.The
newManagerTestServerConfighelper setsExposedAddressto the bare address without the"rel://"prefix, whereasnewClientTestServerConfigin client_test.go includes the prefix. The manager tests then wrap addresses withtoURL()helper (line 464).While this works, consider aligning the approach between the two test files for consistency. Either both should include the prefix in the config, or both should use the
toURL()wrapper pattern.♻️ Align ExposedAddress format with client_test.go
func newManagerTestServerConfig(address string) server.Config { return server.Config{ Meter: otel.Meter(""), - ExposedAddress: address, + ExposedAddress: "rel://" + address, TLSSupport: false, AuthValidator: &allow.Auth{}, } }Then update test calls to use
serverCfg.ExposedAddressdirectly instead oftoURL(lstCfg1)..github/workflows/wasm-build-validation.yml (1)
17-19: Consider narrowing the scope of WASM environment variables.Setting
GOOS=jsandGOARCH=wasmat the job level affects all steps, including checkout, setup-go, and apt install. Most of these steps don't require WASM-specific settings. Consider moving these to the golangci-lint step level (similar to line 51 in the js_build job) to avoid potential unexpected behavior.♻️ Suggested refactor
- env: - GOOS: js - GOARCH: wasm steps: - name: Checkout repository uses: actions/checkout@v4 - name: Install Go uses: actions/setup-go@v5 with: go-version-file: "go.mod" - name: Install dependencies run: sudo apt update && sudo apt install -y -q libgtk-3-dev libayatana-appindicator3-dev libgl1-mesa-dev xorg-dev libpcap-dev - name: Install golangci-lint uses: golangci/golangci-lint-action@4afd733a84b1f43292c63897423277bb7f4313a9 # v8.0.0 with: version: latest install-mode: binary skip-cache: true skip-save-cache: true cache-invalidation-interval: 0 working-directory: ./client + env: + GOOS: js + GOARCH: wasm continue-on-error: true
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (77)
.devcontainer/Dockerfile.github/workflows/golang-test-freebsd.yml.github/workflows/golang-test-linux.yml.github/workflows/golangci-lint.yml.github/workflows/release.yml.github/workflows/wasm-build-validation.yml.golangci.yamlclient/cmd/debug.goclient/cmd/login.goclient/cmd/pprof.goclient/cmd/root.goclient/cmd/status.goclient/cmd/testutil_test.goclient/cmd/up.goclient/firewall/iptables/acl_linux.goclient/firewall/iptables/manager_linux_test.goclient/firewall/nftables/manager_linux_test.goclient/firewall/uspfilter/filter.goclient/firewall/uspfilter/localip.goclient/firewall/uspfilter/localip_test.goclient/firewall/uspfilter/nat_test.goclient/iface/device/device_ios.goclient/internal/debug/debug_linux.goclient/internal/iface.goclient/internal/routemanager/iface/iface.goclient/internal/routemanager/systemops/systemops_generic.goclient/ios/NetBirdSDK/client.goclient/server/panic_windows.goclient/ssh/server/jwt_test.goclient/system/info_android.goclient/system/info_darwin.goclient/system/info_ios.goclient/ui/client_ui.goclient/ui/signal_windows.gogo.modmanagement/cmd/management.gomanagement/internals/shared/grpc/loginfilter_test.gomanagement/server/account.gomanagement/server/account_test.gomanagement/server/http/handlers/policies/posture_checks_handler_test.gomanagement/server/http/testing/benchmarks/peers_handler_benchmark_test.gomanagement/server/http/testing/benchmarks/setupkeys_handler_benchmark_test.gomanagement/server/http/testing/benchmarks/users_handler_benchmark_test.gomanagement/server/http/testing/integration/setupkeys_handler_integration_test.gomanagement/server/idp/pocketid.gomanagement/server/idp/zitadel.gomanagement/server/migration/migration.gomanagement/server/nameserver.gomanagement/server/posture_checks.gomanagement/server/store/sql_store_get_account_test.gomanagement/server/store/sql_store_test.gomanagement/server/testutil/store.gomanagement/server/testutil/store_ios.gorelay/cmd/pprof.gorelay/server/listener/quic/conn.gorelay/server/listener/ws/conn.goshared/management/client/rest/accounts_test.goshared/management/client/rest/client.goshared/management/client/rest/client_test.goshared/management/client/rest/dns_test.goshared/management/client/rest/events_test.goshared/management/client/rest/geo_test.goshared/management/client/rest/groups_test.goshared/management/client/rest/impersonation_test.goshared/management/client/rest/networks_test.goshared/management/client/rest/peers_test.goshared/management/client/rest/policies_test.goshared/management/client/rest/posturechecks_test.goshared/management/client/rest/routes_test.goshared/management/client/rest/setupkeys_test.goshared/management/client/rest/tokens_test.goshared/management/client/rest/users_test.goshared/relay/client/client_test.goshared/relay/client/dialer/quic/conn.goshared/relay/client/manager_test.gosignal/cmd/run.goutil/syslog_nonwindows.go
💤 Files with no reviewable changes (32)
- client/system/info_android.go
- shared/management/client/rest/dns_test.go
- management/server/http/testing/benchmarks/peers_handler_benchmark_test.go
- management/server/http/testing/integration/setupkeys_handler_integration_test.go
- shared/management/client/rest/groups_test.go
- client/internal/iface.go
- shared/management/client/rest/tokens_test.go
- shared/management/client/rest/networks_test.go
- shared/management/client/rest/geo_test.go
- client/system/info_darwin.go
- shared/management/client/rest/accounts_test.go
- shared/management/client/rest/impersonation_test.go
- util/syslog_nonwindows.go
- client/iface/device/device_ios.go
- shared/management/client/rest/posturechecks_test.go
- shared/management/client/rest/policies_test.go
- client/internal/routemanager/iface/iface.go
- shared/management/client/rest/client_test.go
- shared/management/client/rest/peers_test.go
- management/server/testutil/store.go
- client/cmd/pprof.go
- client/server/panic_windows.go
- management/server/http/testing/benchmarks/users_handler_benchmark_test.go
- relay/cmd/pprof.go
- shared/management/client/rest/events_test.go
- shared/management/client/rest/setupkeys_test.go
- shared/management/client/rest/users_test.go
- shared/management/client/rest/routes_test.go
- client/cmd/testutil_test.go
- client/system/info_ios.go
- management/server/testutil/store_ios.go
- management/server/http/testing/benchmarks/setupkeys_handler_benchmark_test.go
🧰 Additional context used
🧬 Code graph analysis (10)
client/ui/signal_windows.go (1)
shared/management/status/error.go (1)
Errorf(70-75)
management/server/account.go (1)
management/server/idp/idp.go (1)
AppMetadata(96-102)
relay/server/listener/quic/conn.go (1)
shared/relay/client/dialer/quic/conn.go (2)
Conn(32-35)NewConn(37-42)
shared/relay/client/client_test.go (3)
shared/relay/auth/hmac/store.go (1)
TokenStore(13-16)shared/relay/client/client.go (1)
NewClient(149-170)client/iface/iface.go (1)
DefaultMTU(28-28)
shared/relay/client/dialer/quic/conn.go (1)
relay/server/listener/quic/conn.go (2)
Conn(14-20)NewConn(22-29)
management/server/store/sql_store_test.go (1)
route/route.go (5)
Route(91-112)ID(48-48)NetID(54-54)NetworkType(60-60)IPv4Network(40-40)
signal/cmd/run.go (1)
util/wsproxy/constants.go (2)
ProxyPath(6-6)SignalComponent(11-11)
shared/management/client/rest/client.go (1)
shared/management/status/error.go (1)
Errorf(70-75)
management/server/http/handlers/policies/posture_checks_handler_test.go (1)
shared/management/status/error.go (2)
Errorf(70-75)InvalidArgument(27-27)
management/server/posture_checks.go (1)
shared/management/status/error.go (3)
Errorf(70-75)InvalidArgument(27-27)Error(54-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Client / Unit
- GitHub Check: release_ui
- GitHub Check: release_ui_darwin
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
- GitHub Check: release
- GitHub Check: test-docker-compose (postgres)
- GitHub Check: Build Cache
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Android / Build
- GitHub Check: test-getting-started-script
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: Darwin
- GitHub Check: test-docker-compose (sqlite)
- GitHub Check: iOS / Build
- GitHub Check: test-docker-compose (mysql)
- GitHub Check: Client / Unit
- GitHub Check: Check External GPL/AGPL Licenses
🔇 Additional comments (45)
shared/management/client/rest/client.go (1)
164-164: LGTM! Error messages now follow Go conventions.The lowercase formatting aligns with Go's style guidelines that error strings should not be capitalized (unless starting with proper nouns or acronyms).
Also applies to: 172-172
client/firewall/iptables/acl_linux.go (1)
390-390: LGTM! Clean simplification.The refactor correctly condenses the IP-match flag logic into a single, clear expression. The negation of
ip.IsUnspecified()directly implements the comment on line 389 and maintains the same behavior in a more idiomatic way.client/internal/debug/debug_linux.go (1)
506-524: LGTM! Clean refactoring to switch statements.The consolidation of the if-else chains into switch statements with multi-value cases (
case 4, 2) is a good improvement. The behavior is preserved while making the code more concise and idiomatic.management/server/store/sql_store_test.go (4)
112-123: LGTM! Route type refactoring applied correctly.The route initialization properly uses the
nbroutepackage types with explicit type conversions forID,NetID, andNetworkType. The test logic remains unchanged.
546-558: LGTM! Consistent type refactoring.The route initialization correctly uses
nbroute.Routeandnbroute.IPv4Network, maintaining consistency with the package refactoring throughout the test file.
690-701: LGTM! Migration test updated correctly.The local route type definition and initialization properly use
nbroute.Route, ensuring the migration test validates compatibility between old and new route type representations.
716-720: LGTM! Migration test route initialization correct.The route initialization properly uses
nbroute.Routetype, maintaining consistency with the package refactoring and ensuring migration tests validate the new type structure.client/firewall/uspfilter/localip.go (1)
133-134: LGTM - Valid suppression of false positive.The nosec directive correctly suppresses a G602 warning for a loop that has valid bounds (0-8191 for an array of size 8192). The inline comment clearly explains the suppression rationale.
client/firewall/uspfilter/filter.go (1)
798-798: LGTM - Clean type inference.Removing the explicit type annotation is appropriate here since
pseudoSumis alreadyuint32, making the type inference straightforward and the code slightly cleaner.client/firewall/uspfilter/nat_test.go (1)
237-242: LGTM - More idiomatic branching.The switch statement is a cleaner and more idiomatic approach for branching on protocol type. The test logic and assertions remain functionally equivalent.
client/firewall/uspfilter/localip_test.go (1)
221-221: LGTM - Simplified blank identifier usage.The simplified form
_ = mapManager.localIPs[...]is cleaner while still achieving the same benchmark behavior. Thenolint:gosimpledirective appropriately addresses the linter feedback.Also applies to: 230-230
client/firewall/iptables/manager_linux_test.go (1)
164-164: LGTM - Clean type inference refactor.The switch to type inference for index variables is a minor style improvement. The variables are correctly inferred as
intfrom the-1literals, maintaining identical functionality while being more concise.client/firewall/nftables/manager_linux_test.go (3)
201-201: LGTM - Clean type inference refactor.Same as the iptables test file, the type inference correctly infers
intfrom the-1literals with no functional change.
211-217: LGTM - More idiomatic control flow.The refactor from if-else to switch statement for
lookup.SetNamematching is a clean improvement. The switch statement is more idiomatic Go for checking discrete string values and maintains identical functionality.
227-232: LGTM - More idiomatic control flow.Same improvement as the SetName check above—the switch statement is more idiomatic for checking
verdict.Kindvalues and preserves the exact same matching logic.management/server/migration/migration.go (1)
396-396: Consistency improvement verified.The change from
db.Dialector.Name()todb.Name()correctly aligns with the existing pattern used in theGetColumnNamefunction (line 22). GORM v1.25.12 supportsdb.Name()as a convenience method, and no other instances ofdb.Dialector.Name()remain in the codebase to update.management/server/idp/zitadel.go (1)
360-360: LGTM! Idiomatic Go improvement.The change from explicit variable declaration to short variable declaration is more concise and follows Go conventions.
management/server/nameserver.go (1)
23-23: LGTM! Better naming convention.Renaming
invalidDomainNametoerrInvalidDomainNamefollows the Go convention of prefixing error variables witherr, improving code readability.Also applies to: 317-317
management/server/idp/pocketid.go (2)
124-124: LGTM! Consistent error message formatting.Lowercasing the error message follows the Go convention that error strings should not be capitalized (unless beginning with proper nouns or acronyms).
304-304: LGTM! Idiomatic Go improvement.The change from explicit variable declaration to short variable declaration is more concise and follows Go conventions.
management/server/http/handlers/policies/posture_checks_handler_test.go (1)
49-49: LGTM! Correct error formatting.The change to use
%vwitherrdirectly is idiomatic Go and correctly mirrors the formatting approach in the source code.client/ui/signal_windows.go (1)
167-167: LGTM! Error message follows Go conventions.Lowercasing the error message aligns with Go's style guidelines, as error strings are typically not capitalized (unless starting with proper nouns or acronyms) since they're often wrapped with additional context.
client/ui/client_ui.go (1)
513-513: Error message capitalization follows Go conventions.The lowercase error messages at these lines align with Go's official style guidance, which recommends keeping error strings lowercase to support clean composition when errors are wrapped. The broader codebase consistently follows this pattern, including properly capitalizing acronyms like MTU, SSH, and JWT within the lowercase messages.
While user-facing dialogs could benefit from sentence-level capitalization for UX, Go convention prioritizes the lowercase approach to maintain consistency across the codebase. Given this appears to be a revert aligning the code with standard practices, the changes are appropriate.
client/internal/routemanager/systemops/systemops_generic.go (2)
213-251: LGTM! Switch refactoring improves readability.The refactoring from if-else chains to a switch statement is well-executed. The logic is correctly preserved:
- Default IPv4/IPv6 routes are split into /1 prefixes to avoid replacing existing routes
- Rollback logic is maintained for error cases
- Non-default prefixes fall through to direct route table addition
259-290: Default case correctly handles non-default prefixes. The switch refactoring maintains proper symmetry withgenericAddVPNRoute—both functions delegate non-default prefixes through their default cases toremoveFromRouteTableandaddToRouteTablerespectively. Test coverage inTestAddVPNRouteconfirms that non-default routes (private networks, host routes, and custom subnets) are properly added and removed without leaks.management/cmd/management.go (1)
67-70: LGTM!The change from short variable declaration to explicit
vardeclaration is a valid stylistic adjustment. The logic correctly initializestlsEnabledtofalseand sets it totruewhen TLS configuration is present.relay/server/listener/ws/conn.go (1)
87-92: LGTM!Using
c.CloseNow()instead ofc.Conn.CloseNow()is the idiomatic Go pattern for calling promoted methods from embedded types. This improves readability while maintaining identical behavior..github/workflows/release.yml (1)
66-69: Go version updated to 1.25.5 for FreeBSD release builds.The Go toolchain update aligns with the PR objectives. Note that this uses a different patch version (1.25.5) than the FreeBSD test workflow (1.25.3). If both workflows should use the same version, consider aligning them.
management/server/account_test.go (1)
756-758: Verify nil-guard removal is intentional.The nil-guard checks (
account != nil &&) were removed from these conditions. While the earlier nil checks at lines 752 and 767 should prevent nil dereference, defensive programming in tests typically keeps such guards to make the code more resilient to future refactoring.Verify this removal is intentional and won't cause issues if the test structure changes in the future.
Also applies to: 771-773
signal/cmd/run.go (2)
76-79: LGTM: Idiomatic variable declaration.The change from
tlsEnabled := falsetovar tlsEnabled boolis a good style improvement, making it clearer that the zero value is intentionally used.
262-267: LGTM: Switch statement improves clarity and extensibility.The refactor from if-else to a switch statement is a good improvement. It's more idiomatic and makes it easier to add additional URL path handlers in the future.
relay/server/listener/quic/conn.go (1)
15-28: LGTM: Consistent with QUIC library upgrade.The type changes from
quic.Connectionto*quic.Connare consistent with the corresponding changes in the client-side code (shared/relay/client/dialer/quic/conn.go). This coordinated update across both client and server components confirms it's part of the quic-go v0.55.0 library upgrade.shared/relay/client/dialer/quic/conn.go (1)
33-41: Type change aligns correctly with quic-go v0.55.0 upgrade.The change from
quic.Connectioninterface to*quic.Connconcrete type is required for v0.55.0, where the Connection interface was removed in the v0.53.0 API change. The call site inshared/relay/client/dialer/quic/quic.go:69correctly passes the result ofquic.Dial(), which returns*quic.Connin this version. All method calls on the session object (ReceiveDatagram,SendDatagram,RemoteAddr,LocalAddr,CloseWithError) are valid on the concrete*quic.Conntype.shared/relay/client/client_test.go (2)
31-39: Good refactor: Centralized test server configuration.The new helper function
newClientTestServerConfigcentralizes server configuration for tests, improving maintainability and consistency. The function creates a properly configured server with the relay protocol prefix and standard test components (meter, auth validator).
43-44: Port allocation is properly managed by CI configuration.The concern about port conflicts from parallel test execution is not applicable here. The CI workflows explicitly run tests sequentially using the
-p 1flag in thego testcommand (seegolang-test-linux.ymlandgolang-test-darwin.yml), which prevents tests from running in parallel. Additionally, all test servers bind to127.0.0.1(loopback only), which prevents conflicts with other services. Each test uses a unique port (50001, 50101, 50201, etc.), following a consistent pattern. No action required.Likely an incorrect or invalid review comment.
.golangci.yaml (2)
1-23: Good migration: golangci-lint v2 configuration format.The linter configuration has been successfully migrated to the v2 format with proper structure. The explicit
default: nonewith an enabled list provides clear visibility into which linters are active.
70-115: Exclusions syntax verified as correct for golangci-lint v2.The exclusions have been properly migrated to the v2 format. All fields are correctly structured:
generatedset tolax, standard presets included (comments,common-false-positives,legacy,std-error-handling), andrulesarray with properlinters,path, andtextfields. Thepathsarray for broad exclusions is also correctly formatted. No syntax issues found..github/workflows/golang-test-linux.yml (2)
262-262: Good addition: Parallelism limit for relay tests.Adding the
-p 1flag to limit test parallelism is a good practice for relay tests running with sudo privileges in a containerized environment. This helps prevent race conditions and resource contention.
203-203: golang:1.25-alpine image is available and stable.Go 1.25 was officially released on August 12, 2025, and the corresponding
golang:1.25-alpineDocker image is publicly available on Docker Hub. The image has been stable for over four months and is safe to use in CI/CD workflows.go.mod (3)
108-108: go.uber.org/mock v0.5.2 is available and valid.The version v0.5.2 is a legitimate public release (published April 28, 2025) and represents a valid upgrade path from v0.5.0 (released October 15, 2024). No security advisories were identified for this version.
3-5: Go 1.25.5 toolchain is available and stable.Go 1.25.5 is an official upstream security/patch release with binaries and source available from go.dev/dl. It follows the Go 1 compatibility promise, ensuring existing Go 1.25 programs will continue to build and run. The toolchain is widely available across major package ecosystems (Microsoft, Fedora, and other Linux distributions) and CI/CD environments.
86-86: Update quic-go to v0.55.0, ensuring Go version compatibility.The QUIC library upgrade to v0.55.0 does not include explicit breaking API changes. The primary compatibility requirement is support for Go 1.24 and 1.25. The upgrade also includes a security fix for CVE-2025-59530 (client DoS via premature HANDSHAKE_DONE frame), though this was also included in v0.49.1. Verify that the project's Go version requirements align with the minimum supported version for this update.
Likely an incorrect or invalid review comment.
.github/workflows/golangci-lint.yml (2)
55-55: Consistency with wasm-build-validation.yml workflow.This uses the same golangci-lint-action commit hash as the wasm-build-validation.yml workflow. Verification of the v8.0.0 tag should cover both files.
58-61: Cache disabling will impact all matrix builds.The cache is now completely disabled across all three OS runners (macOS, Windows, Linux) in the matrix. This may significantly increase CI execution time since cache benefits won't be available for any platform. Verify this is intentional to address cache-related issues that outweigh the performance cost.
The simplified
args: --timeout=12mremoves the previous output formatting option, which should work with the latest golangci-lint-action version..github/workflows/wasm-build-validation.yml (1)
35-37: No action needed; working-directory and cache settings are intentional.The
working-directory: ./clientrestriction is appropriate since the WASM build explicitly targets./client/wasm/cmdand all WASM-specific code (identified by_js.gofiles) resides within the client directory. Broader codebase linting is handled by the separategolangci-lint.ymlworkflow, which runs without directory restrictions on multiple OS platforms. The cache disabling is also intentional and consistent across all linting workflows.Likely an incorrect or invalid review comment.
| GO_TARBALL="go1.25.3.freebsd-amd64.tar.gz" | ||
| GO_URL="https://go.dev/dl/$GO_TARBALL" | ||
| curl -vLO "$GO_URL" | ||
| tar -C /usr/local -vxzf "$GO_TARBALL" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the workflow files mentioned
find .github/workflows -name "*.yml" -o -name "*.yaml" | head -20Repository: netbirdio/netbird
Length of output: 707
🏁 Script executed:
# Check the specific file mentioned in the review
cat -n .github/workflows/golang-test-freebsd.yml | sed -n '25,35p'Repository: netbirdio/netbird
Length of output: 536
🏁 Script executed:
# Search for release.yml and check its Go version
find .github -name "*release*" -type fRepository: netbirdio/netbird
Length of output: 90
🏁 Script executed:
# Search for all Go version references in workflow files
rg "go[0-9]\.[0-9]+\.[0-9]+" .github/workflows/ -oRepository: netbirdio/netbird
Length of output: 150
🏁 Script executed:
# Check release.yml for Go version context
cat -n .github/workflows/release.yml | grep -B5 -A5 "1.25.5"Repository: netbirdio/netbird
Length of output: 532
🏁 Script executed:
# Check all workflow files for Go version references more comprehensively
rg "go1\.25\." .github/workflows/ -B2 -A2Repository: netbirdio/netbird
Length of output: 803
🌐 Web query:
Go 1.25.3 Go 1.25.5 release available download
💡 Result:
Yes — both Go 1.25.3 and Go 1.25.5 binary and source downloads are available on the official Go downloads page. See the download listings for go1.25.5 (and other 1.25.x releases) on the Go site. [1][2]
Sources:
- Official Go downloads page (lists go1.25.5 and 1.25.x artifacts). [1]
- Go release history / release notes (go1.25.3 and go1.25.5 entries). [2]
(If you want direct download links or checksums for a specific OS/arch, tell me which one.)
Align Go versions across workflows: golang-test-freebsd.yml uses go1.25.3 while release.yml uses go1.25.5.
Both versions exist and are available, but this inconsistency should be resolved. Either align both workflows to use the same version (likely go1.25.5 as the more recent patch) or document why the versions differ.
🤖 Prompt for AI Agents
In @.github/workflows/golang-test-freebsd.yml around lines 28 - 31, The workflow
sets GO_TARBALL/GO_URL to "go1.25.3.freebsd-amd64.tar.gz"/"$GO_TARBALL" which
mismatches release.yml's go1.25.5; update the GO_TARBALL and any references to
use go1.25.5 (e.g., "go1.25.5.freebsd-amd64.tar.gz" and corresponding GO_URL) so
both workflows use the same Go patch release, or add a comment/documentation
near the GO_TARBALL/GO_URL variables explaining and justifying the intentional
version difference.
| run: sudo apt update && sudo apt install -y -q libgtk-3-dev libayatana-appindicator3-dev libgl1-mesa-dev xorg-dev libpcap-dev | ||
| - name: Install golangci-lint | ||
| uses: golangci/golangci-lint-action@d6238b002a20823d52840fda27e2d4891c5952dc | ||
| uses: golangci/golangci-lint-action@4afd733a84b1f43292c63897423277bb7f4313a9 # v8.0.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the commit hash for v8.0.0 tag
cd /tmp && rm -rf golangci-lint-action
git clone --depth 100 https://github.com/golangci/golangci-lint-action.git 2>&1 | grep -E "(Cloning|done)"
cd golangci-lint-action
git fetch origin tag v8.0.0 --no-tags 2>&1 | grep -E "(Fetching|error|fatal)"
echo "=== v8.0.0 tag commit hash ==="
git rev-parse v8.0.0
echo "=== Expected commit hash ==="
echo "4afd733a84b1f43292c63897423277bb7f4313a9"
echo "=== Match result ==="
if [ "$(git rev-parse v8.0.0)" = "4afd733a84b1f43292c63897423277bb7f4313a9" ]; then
echo "✓ MATCH: Hash corresponds to v8.0.0"
else
echo "✗ MISMATCH: Hash does not correspond to v8.0.0"
fiRepository: netbirdio/netbird
Length of output: 309
🏁 Script executed:
#!/bin/bash
# Find what tag/version the actual commit hash belongs to
cd /tmp/golangci-lint-action
echo "=== Tags for commit 4afd733a84b1f43292c63897423277bb7f4313a9 ==="
git tag --contains 4afd733a84b1f43292c63897423277bb7f4313a9 2>/dev/null | head -20
echo ""
echo "=== Commit date and message ==="
git log -1 --format="%H %ai %s" 4afd733a84b1f43292c63897423277bb7f4313a9
echo ""
echo "=== All tags in repo (last 20) ==="
git tag --sort=-version:refname | head -20Repository: netbirdio/netbird
Length of output: 352
Update golangci-lint-action to pin to the actual v8.0.0 commit or correct the version comment.
The commit hash 4afd733a84b1f43292c63897423277bb7f4313a9 does not correspond to the v8.0.0 release. The actual v8.0.0 tag points to commit 25e2cdc5eb1d7a04fdc45ff538f1a00e960ae128. The pinned commit predates the v8.0.0 release and should either be updated to the correct v8.0.0 commit or the version comment should be corrected to reflect the actual version being used.
🤖 Prompt for AI Agents
In @.github/workflows/wasm-build-validation.yml at line 30, The workflow pins
golangci-lint-action to commit 4afd733a84b1f43292c63897423277bb7f4313a9 while
commenting v8.0.0; update the uses entry to point to the actual v8.0.0 commit
hash 25e2cdc5eb1d7a04fdc45ff538f1a00e960ae128 (i.e., replace the existing commit
in the uses value) or, if you intentionally want a different commit, change the
trailing comment to the correct version tag to avoid the mismatch.
| func doDaemonLogin(ctx context.Context, cmd *cobra.Command, providedSetupKey string, activeProf *profilemanager.Profile, username string, pm *profilemanager.ProfileManager) error { | ||
| conn, err := DialClientGRPCServer(ctx, daemonAddr) | ||
| if err != nil { | ||
| //nolint |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n client/cmd/login.go | sed -n '75,95p'Repository: netbirdio/netbird
Length of output: 886
🏁 Script executed:
rg -n "//nolint" --type=go -C 2 | head -100Repository: netbirdio/netbird
Length of output: 6128
🏁 Script executed:
fd -e "golangci.yml" -e ".golangci.yml" -e ".golangci.yaml" -e "golangci.yaml" -e ".lintignore"Repository: netbirdio/netbird
Length of output: 43
🏁 Script executed:
rg -n "fmt\.Errorf" --type=go -C 1 | grep -A 2 -B 2 "nolint" | head -50Repository: netbirdio/netbird
Length of output: 1432
Use a specific nolint directive to clarify the suppressed rule.
The generic //nolint directive suppresses all lint warnings on this line. Consider replacing it with a more specific directive like //nolint:lll (for line length) to make the intent clear and prevent accidental suppression of other violations. This pattern appears throughout the codebase (e.g., client/cmd/root.go:393, client/cmd/up.go:219), so standardizing on a specific rule would improve code clarity.
🤖 Prompt for AI Agents
In @client/cmd/login.go at line 84, Replace the generic `//nolint` on the line
containing the `//nolint` token with a rule-specific nolint (e.g.,
`//nolint:lll` for long lines) so only the intended linter check is suppressed;
identify the exact lint being avoided by running your linter or checking similar
instances (e.g., `client/cmd/root.go` and `client/cmd/up.go` use `//nolint:lll`)
and change the comment to `//nolint:<rule>` to narrowly scope the suppression.
2 participants
Reverts 55055
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
Chores
Style
✏️ Tip: You can customize this high-level summary in your review settings.